fix: carry the IDE entry's env when wiring the datamate stdio MCP server - #1081
Conversation
`datamate_manager add` reused the command + args from the IDE's `mcp.json` `datamate` entry but dropped its `env` block, both in the immediate spawn and in the entry persisted to `.altimate-code/altimate-code.json`. On desktop editors the command is the editor's Electron binary and `env` carries `ELECTRON_RUN_AS_NODE=1` — spawned without it, the editor GUI boots and opens `datamate-cli.js` as a document, the MCP client reports `-32000 Connection closed`, and the broken persisted entry re-pops the file on every subsequent session launch. - `readDatamateTransportFromIde` now returns the entry's env (minus `ALTIMATE_EXTENSION_RPC`, mirroring the sync path) and `updatedAt`; `handleAdd` carries the env into the runtime config and persists it as `environment`, plus `updatedAt` on disk so the sync recognizes the entry as current. - The sync path's inline env-strip is extracted into the shared `extractSpawnEnvironment` helper so both paths stay in lockstep. - The TUI worker and `run` now run `syncDatamateUrlFromVscodeMcp` before the first session (as `serve` already did), so entries already persisted without `environment` self-heal on the next launch.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDatamate local transports now preserve filtered environment variables and ChangesDatamate synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RunCommand
participant DatamateTransport
participant MCPConfig
participant DatamateGateway
RunCommand->>DatamateTransport: resolve project root
RunCommand->>MCPConfig: synchronize Datamate entry
MCPConfig->>DatamateTransport: read command, environment, and updatedAt
DatamateTransport-->>MCPConfig: return filtered transport metadata
MCPConfig->>DatamateGateway: persist refreshed entry
DatamateGateway-->>RunCommand: complete or suppress synchronization error
RunCommand->>DatamateGateway: start local session
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit keeps the Node flag bright, Comment |
github-actions
Bot
added
needs:compliance
contributor
needs:issue
and removed
needs:compliance
labels
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
This PR fixes a desktop-editor regression where the IDE-provided datamate stdio MCP entry’s env (notably ELECTRON_RUN_AS_NODE=1) was dropped when wiring/persisting the server, causing Electron to boot the editor UI and open datamate-cli.js as a tab, and leading to -32000 Connection closed. It also expands the “heal from .vscode/mcp.json” sync behavior so terminal entrypoints (run/TUI worker) self-repair already-persisted broken entries, matching serve startup behavior.
Changes:
- Carry the IDE
env(minusALTIMATE_EXTENSION_RPC) andupdatedAtthroughreadDatamateTransportFromIde,datamate_manager addruntime wiring, and persisted config. - Deduplicate env-stripping logic into a shared
extractSpawnEnvironment()helper to keepaddand sync paths aligned. - Trigger
syncDatamateUrlFromVscodeMcpearlier forrunand the TUI worker so previously-broken persisted entries self-heal on next launch.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/src/altimate/datamate-transport.ts | Adds env + updatedAt propagation for IDE datamate stdio entries; factors env normalization into extractSpawnEnvironment; updates sync to use shared env extraction. |
| packages/opencode/src/altimate/tools/datamate.ts | Ensures datamate_manager add carries environment into runtime MCP config and persists environment + updatedAt to disk. |
| packages/opencode/src/cli/tui/worker.ts | Adds a boot-time datamate sync gate so the worker doesn’t serve requests / start external server mode until the heal attempt finishes. |
| packages/opencode/src/cli/cmd/run.ts | Runs the same datamate sync before bootstrapping a session to self-heal env-less persisted entries. |
| packages/opencode/test/release-validation/mcp-datamate-stdio-env.test.ts | Adds regression coverage for env carry-through, stripping rules, back-compat, and sync parity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (17 snapshots, latest commit 63b489a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 63b489a)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit b83a8cb)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 04daa04)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit bdce412)Status: No Issues Found | Recommendation: Merge Incremental review of The prior review's only SUGGESTION is resolved: the connected-entry provenance stamp and the disconnected refresh path now share a single Files Reviewed (3 files)
Previous review (commit 01124f1)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit e96f26b)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 692417a)Status: No Issues Found | Recommendation: Merge Incremental review of The commit resolves the prior review's only SUGGESTION: the duplicated blank-tombstone predicate is now a single Files Reviewed (1 file)
Previous review (commit 9b16734)Status: 1 Issue Found | Recommendation: Address before merge Incremental review of The extension blanks Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 7f68428)Status: No Issues Found | Recommendation: Merge Incremental review of Test-only change: the adversarial Files Reviewed (1 file)
Previous review (commit 42311f2)Status: No Issues Found | Recommendation: Merge Incremental review of The split is correct: Files Reviewed (2 files)
Previous review (commit 6625177)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Previous review (commit 33b60d8)Status: 1 Issue Found | Recommendation: Merge (1 non-blocking suggestion) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
The incremental commit ( Fix these issues in Kilo Cloud Previous review (commit 37c3d2c)Status: No Issues Found | Recommendation: Merge The incremental commit ( Files Reviewed (1 file)
Previous review (commit 7bcc9b6)Status: 1 Issue Found | Recommendation: Merge (non-blocking suggestion) Overview
Issue Details (click to expand)SUGGESTION
The incremental commit (
Only a minor DRY suggestion remains. Fix these issues in Kilo Cloud Files Reviewed (3 files)
Previous review (commit 1cb8fad)Status: No Issues Found | Recommendation: Merge The incremental commit ( Behavior is verified identical at both call sites:
No new issues introduced; the Files Reviewed (2 files)
Previous review (commit 80d4ad4)Status: 1 Issue Found | Recommendation: Merge (non-blocking) Overview
The incremental changes (commit Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit cbf4f65)Status: No Issues Found | Recommendation: Merge The fix correctly carries the IDE Files Reviewed (5 files)
Reviewed by deepseek-v4-pro · Input: 38.5K · Output: 9.4K · Cached: 306K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…root sync scope - TUI worker: the datamate heal is now sequenced strictly before `InstanceRuntime.load`/`Config.get()` (trace init awaits it), so the config read can neither race the non-atomic write nor cache the pre-heal entry — the first session connects with the healed config. - `datamate_manager add`: the in-config-but-not-connected branch refreshes the persisted entry from the current IDE transport (preserving user-managed fields) and connects via `MCP.add`, instead of `MCP.connect` which re-reads the stale in-memory entry. - Boot heals (`run`, TUI worker) scan from the containing git project root via the new `resolveDatamateSyncRoot`, not raw cwd — a session launched from a subdirectory now finds the root IDE config and persisted entry.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
…dd refresh Both paths encode the same idea — entry fields re-derived from the IDE transport versus user-managed fields carried forward. A single exported set keeps them from silently diverging when a new transport field is added; the add-refresh path layers `enabled` on top since it re-derives that too.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…rry updatedAt for remote - The add-refresh path wrote the merged entry (preserved headers/oauth/timeout + fresh transport) to disk but connected the live client with the bare transport config, dropping authentication and connection settings for the session being connected. MCP.add now receives the same merged entry as the disk write, matching the reload-datamate endpoint. - The remote transport variant now carries updatedAt like the local one, so a remote datamate added via datamate_manager is not rewritten once by the next boot's sync purely for the missing change signal.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/altimate/datamate-transport.ts (1)
277-284: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize MCP config writes before this sync path.
addMcpToConfigreadsmcpConfig, then writes withmodify+Filesystem.writewithout a lock. Concurrentdatamate_manager addwrites to the same server can overwrite newer fields such asenvironment,updatedAt, or user-managedheaders/oauth/timeout. Add a per-config-path lock or update queue that covers IDE sync anddatamate_manager add, and keep the lock shared whenresolveConfigPathpoints to the same file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/datamate-transport.ts` around lines 277 - 284, Serialize the read-modify-write flow in addMcpToConfig with a per-config-path lock or update queue covering both IDE synchronization and datamate_manager add operations. Ensure resolveConfigPath results sharing the same file reuse the same lock, and hold it through mcpConfig reads, modify, and Filesystem.write so newer environment, updatedAt, headers, oauth, and timeout fields are not overwritten.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/datamate-transport.ts`:
- Around line 23-24: Update syncDatamateUrlFromVscodeMcp to compare the datamate
entry’s TRANSPORT_IDENTITY_FIELDS whenever readDatamateTransportFromIde returns
a transport without vscodeUpdatedAt, while preserving timestamp-based
synchronization when the timestamp is present. Add a regression test covering a
timestamp-less IDE transport and verifying that altimate-code.json is
synchronized.
---
Outside diff comments:
In `@packages/opencode/src/altimate/datamate-transport.ts`:
- Around line 277-284: Serialize the read-modify-write flow in addMcpToConfig
with a per-config-path lock or update queue covering both IDE synchronization
and datamate_manager add operations. Ensure resolveConfigPath results sharing
the same file reuse the same lock, and hold it through mcpConfig reads, modify,
and Filesystem.write so newer environment, updatedAt, headers, oauth, and
timeout fields are not overwritten.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cbe1fe5-948b-4030-829b-bd6729445c96
📒 Files selected for processing (3)
packages/opencode/src/altimate/datamate-transport.tspackages/opencode/src/altimate/tools/datamate.tspackages/opencode/test/release-validation/mcp-datamate-stdio-env.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/test/release-validation/mcp-datamate-stdio-env.test.ts
- packages/opencode/src/altimate/tools/datamate.ts
|
Re CodeRabbit's outside-diff finding (serialize |
Both the refresh and new-entry branches persisted the transport's updatedAt with the same conditional spread; a single `updatedAtField` above the branch keeps them from drifting, and the disk-only rationale is documented once.
datamate_manager add supports scope "global", so a broken (env-less) datamate entry can live in the global altimate-code.json. It is spawned at session start like any merged config entry — reproducing the editor-tab pop — but the boot heal only rewrote the project config, so the entry never repaired (found by the bug reporter testing the fix: no environment block appeared). syncDatamateUrlFromVscodeMcp now heals every config file carrying a datamate entry via findAllConfigPaths (project, project subdirs, global), reporting the entry once. Sync tests pass an isolated global dir so test runs never touch the developer's real config.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/datamate-transport.ts`:
- Around line 331-332: Resolve the Git project root once at the start of
syncDatamateUrlFromVscodeMcp, then use that root instead of cwd for both
findAllMcpJsonFiles and findAllConfigPaths. Add a direct regression test
invoking syncDatamateUrlFromVscodeMcp from a nested directory and verify
root-level configuration files are synchronized.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: edd49936-8519-4d1c-ac2f-62ff387826f9
📒 Files selected for processing (3)
packages/opencode/src/altimate/datamate-transport.tspackages/opencode/test/release-validation/mcp-datamate-893.test.tspackages/opencode/test/release-validation/mcp-datamate-stdio-env.test.ts
|
Field testing by the bug reporter surfaced a second gap, fixed in 33b60d8: |
…t rejection, correct stale scan docs Three review findings on the provenance rework: - The already-connected early return in `datamate_manager add` skipped the provenance stamp entirely, so a legacy global entry that happened to be connected could never be repaired — the boot heal rejects unstamped global entries and the documented explicit-add remedy was a no-op in exactly that state. The stamp (plus fresh transport) is now persisted to disk before the early return; the live client stays untouched and the healed entry applies from the next session. - The home-root rejection compared lexical paths; a `$HOME` reached through a symlink (or differing Windows casing) slipped past it and made the whole home tree the "project". Both sides are now canonicalized via realpath (case-insensitive on win32), with a regression test using a symlinked OPENCODE_TEST_HOME. - The datamate_manager doc comment still named `.github/copilot/mcp.json` as a scanned location; the transport scan deliberately covers only the extension-written `.vscode`/`.cursor` files (provenance boundary), while generic discovery keeps surfacing the Copilot file for opt-in. The comment now says exactly that. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSk1Tcr4pH3aZRU4tVKrdi
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… merge, tidy docs and win32 test skip - The connected-entry disk update triggered only on provenance mismatch, so a changed IDE transport under matching provenance kept spawning stale command/env — and entries without updatedAt are skipped by the boot sync, making this path their only repair. The write now triggers whenever any transport-identity or provenance field differs from the freshly merged entry. - The connected stamp and the disconnected refresh now share one mergeRefreshedEntry helper (single exclusion set and merge order). - isSamePath moved above resolveDatamateSyncRoot's JSDoc so the resolver keeps its documentation. - The symlinked-home regression test routes through the suite-standard win32 skip (directory symlinks need elevated privileges there). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSk1Tcr4pH3aZRU4tVKrdi
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Review log — claims contractNumbered falsifiable claims for scoped review; a finding is a reproducible trace violating a claim. Instances of the residuals are accepted trade-offs, not findings. Claims
Residuals
Scoped codex rounds run against this comment; outcomes are appended here. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdce4125cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The reload endpoint's read-back scanned only the instance directory's config files plus global, while the sync heals along the launch-directory-to-root walk — a nested instance with the datamate entry in a root-level config healed the file, then skipped the reconnect and reported updated anyway, leaving the live client on the stale transport. The walk is now extracted as collectDatamateHealPaths, shared by the sync and the endpoint so the two can never disagree about where a healed entry lives. Source guard updated; walk-parity regression test added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSk1Tcr4pH3aZRU4tVKrdi
|
@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim. Instances of the disclosed residuals are not findings. A round with no claim violation ends review. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ved root in the heal walk - A connected entry disabled on disk skipped the refresh write (the identity comparison omitted enabled), so an explicit add failed to persist the re-enable and the disable resurrected on restart. enabled joins the comparison. - collectDatamateHealPaths accepts the caller's already-resolved root; the sync passes its own, removing a redundant bounded root walk per boot. The reload endpoint keeps the internal resolution. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSk1Tcr4pH3aZRU4tVKrdi
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Approving — with three items to fix
The rework closes everything raised in the previous round, and I verified each against the code rather than the description:
- Env carry is now a real allowlist.
SPAWN_ENV_ALLOWLISTis a single key, and the test seedsNODE_OPTIONS,LD_PRELOAD,PATHandALTIMATE_EXTENSION_RPCand asserts onlyELECTRON_RUN_AS_NODEsurvives. That is the right shape of test for this. - Provenance gating works as described — a hand-added global entry and one stamped from a different project's
mcp.jsonboth survive a project-local heal byte-identical. parseIdeTransportcloses the url-less-remote bug, with the exact failing fixture as a regression test.collectDatamateHealPathsis shared by the sync and the reload read-back, so the two can no longer disagree about where a healed entry lives.- The scan is pruned and canonicalized via
DiscoveryFiles, and restricted to the extension-written locations. - The new suite is behavioral throughout — real fixtures, real disk assertions.
The claims contract made this reviewable in a way the previous round was not. Three of the nine do not hold as written, and I would like them addressed — but none is a repeat of a prior blocker, and they are all small.
1. TRANSPORT_IDENTITY_FIELDS omits env, so the allowlist can be bypassed on a heal (C2)
The preserved loop carries any legacy env key forward verbatim. At load, config/config.ts:103 does transformed.environment = entry.env, and :104 then overwrites it from entry.environment. So when the healed entry has no environment key — i.e. the IDE entry supplied no allowlisted value, so extractSpawnEnvironment returned undefined and the key was omitted — the preserved env reaches mcp/index.ts:577 and is spread over process.env at spawn.
That is the allowlist being bypassed in precisely the legacy-healing path this PR exists to serve.
Fix: add "env" to TRANSPORT_IDENTITY_FIELDS. Worth a regression test that heals an entry carrying env.NODE_OPTIONS with no environment.
2. resolveDatamateSyncRoot returns $HOME when the launch directory is $HOME (C6)
Executed against the function:
home IS a git repo -> returns $HOME
home is NOT a git repo -> returns $HOME
subdirectory of home -> returns the subdir (what the current test covers)
Filesystem.up yields $HOME/.git; isSamePath correctly refuses it as root; control then falls through to return directory, which is $HOME. With no .git, nothing is yielded and the same fallthrough applies. The guard rejects $HOME as a walked-to root but not as the starting directory, and test:578 asserts only the subdirectory case — so it passes while the boundary is unguarded. Both run and the TUI worker pass process.cwd().
The consequence is the **/mcp.json glob running over the whole home tree, where an unrelated checkout's IDE entry can win source selection. That harm is the same whether home was reached from below or passed in directly.
Fix: the useful invariant is "$HOME is never a scannable project root" — skip the IDE scan and heal in that case, rather than a strict early return (which would lock out a user whose dotfiles project genuinely is $HOME). Add a test for resolveDatamateSyncRoot(home).
3. Scope dedup can downgrade the global config to project scope, skipping the provenance gate (C4)
Executed with launchDir === globalDir:
collectDatamateHealPaths(globalDir, globalDir)
-> scope=project <globalDir>/altimate-code.json
findProjectConfigPaths(launchDir) finds the physical global config first and tags it scope: "project"; findGlobalConfigPaths then finds the same path but seen suppresses it. healEntryInFile receives "project" and never applies the provenance check, so a hand-added global entry is auto-rewritten from a project-local mcp.json — the case C4 exists to prevent.
Fix: classify by canonical ownership (anything at or under the global config dir is global), or collect global paths first and let global win collisions.
Minor / follow-up
collectDatamateHealPathsusesdir.startsWith(rootResolved)— a lexical prefix match with no separator, so/home/u/prowould "contain"/home/u/projects/sub. It cannot trigger today becausediris always derived bypath.dirnamefromlaunchDir, butrootResolved + path.sepis the sturdier form.identityChangedre-listsmanagedBy/sourceMcpJson, whichTRANSPORT_IDENTITY_FIELDSalready contains.TRANSPORT_IDENTITY_FIELDSnow carries provenance fields too, so the name no longer describes its contents.mergeRefreshedEntrycarriesRemote-only fields (headers,headersCommand,oauth) onto alocalentry across a transport-type change. Ignored at decode, but it puts schema-invalid data on disk.- Disk-only fields (
updatedAt,managedBy,sourceMcpJson) still reachMCP.addon the disconnected-refresh path, contradicting the comment that calls them disk-only. sourceMcpJsonis an absolute path, so moving a project or syncing the global config to another machine silently breaks the provenance bond and drops back to the documented R1 behavior. Worth adding to the residuals list so the support path is known.- The adversarial assertion in
upi-config-mcp.test.ts:199-205still matches on source text and was updated to track the refactor. Pre-existing pattern, but it now guards a security-relevant path, so converting it to a behavior test (seed a global-only entry, hit the reload endpoint, assert whatMCP.addreceives) would be worth more than the string match.
Approving on the strength of the rework; please pick these up before or shortly after merge.
…ot, global scope downgrade - Legacy `env` joins TRANSPORT_IDENTITY_FIELDS: config load aliases `env` to `environment` when no `environment` key is present, so a preserved legacy `env` on a healed entry reached the spawn spread and bypassed the allowlist exactly on the legacy entries the heal exists to repair. Regression test heals an entry carrying env.NODE_OPTIONS and asserts both keys are gone. - A launch root that IS the home directory now skips the heal entirely: the resolver's home rejection only covered walked-to roots, so launching from $HOME globbed the whole home tree where any unrelated checkout's IDE entry could win source selection. Only the automatic heal declines; explicit add still works for a dotfiles project at home. - Heal candidates are scoped by canonical ownership, not discovery order: a file at or under the global config dir is global no matter which loop found it, so launching from that dir can no longer downgrade the physical global config to project scope and skip the provenance gate. The root containment check also uses a separator-anchored prefix. - Folded minors from the same review: duplicate provenance keys removed from the connected-refresh comparison; disk-only fields (updatedAt, managedBy, sourceMcpJson) stripped from the config handed to MCP.add. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSk1Tcr4pH3aZRU4tVKrdi
|
Thank you for executing the claims rather than reading the descriptions — all three findings verified exactly as your traces show, fixed in 63b489a:
Folded from your minors in the same commit: separator-anchored root containment, duplicate provenance keys removed from the connected-refresh comparison, and disk-only fields stripped from the config handed to Deferred, with reasons: the Your four 08-26 threads: resolving them on the strength of your "closes everything raised in the previous round" — shout if you'd rather any stay open. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
A symlink alias (or Windows casing) of the global config dir made the lexical ownership check tag the physical global config as project scope, skipping the provenance gate. Ownership is now decided on canonical paths via the shared canonicalizer that already backs the home-root rejection; regression test launches from the real dir with the alias configured as the global dir. The root-containment expression also drops its redundant clause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSk1Tcr4pH3aZRU4tVKrdi
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Approving — all three findings verified fixed
I re-ran the same probes that produced the original traces, against this head rather than reading the fix descriptions. All three behave correctly now:
Legacy env alias (C2) — persisted entry carrying env: {NODE_OPTIONS: "--require /tmp/payload.js"}, healed against an IDE entry with no allowlisted key:
healed entry keys: type, command, updatedAt, managedBy, sourceMcpJson
env present? no
environment present? no
Home as scannable root (C6) — the sync now declines before scanning, in both the git and non-git cases:
home IS git repo: sync from home -> updated=[] config unchanged? YES (declined)
home NOT git repo: sync from home -> updated=[] config unchanged? YES (declined)
Keeping resolveDatamateSyncRoot(home) === home as a documented fallback and enforcing the invariant at the sync boundary is the better shape — a dotfiles project at home keeps the explicit add path.
Global scope downgrade (C4) — launching from the global config dir, with an unstamped hand-added entry:
scope=global ~/.config/altimate-code/altimate-code.json
hand-added global entry byte-identical after heal? YES (protected)
Previously this printed scope=project.
The three folded minors are all in: separator-anchored containment, the deduplicated identityChanged comparison, and disk-only fields stripped before MCP.add so that comment is finally accurate. The four new tests are behavioral and would fail against the previous head, which is the property that matters. I tried to break the new ownership check (symlinked global dir, global dir nested in the project, launch dir at the global dir) and the home decline (symlinked home, home with and without .git) without success.
Two follow-ups — neither blocks this
1. The containment fix I suggested last round has a filesystem-root edge. !dir.startsWith(rootResolved + path.sep): when rootResolved is /, the prefix becomes "//", which nothing matches, so the ancestor walk stops at the launch directory.
rootResolved='/' dir='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/repo/pkg' -> walks ancestors? false
rootResolved='/repo' dir='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/repo/pkg' -> walks ancestors? true
The previous unanchored form handled / correctly, so anchoring it traded one edge for another — that is on my earlier suggestion, not on you. path.relative(rootResolved, dir) handles both: contained when the result is neither absolute nor ..-prefixed. Reaching it needs .git at / or a launch at /, so it is not urgent, but a POSIX-root case in the containment tests would pin it.
2. The home decline also stops the non-datamate remote URL refresh. The early return sits before the transport scan, so the second pass that refreshes URLs for other remote MCP entries never runs from home:
launched from HOME:
updated = []
othersrv URL refreshed? NO
Reasonable as written — both passes go through findAllMcpJsonFiles(root), and the recursive home scan is exactly what the guard prevents. Worth noting only because that pass is documented as deliberate behavior, and it is recoverable without reopening the hole: reading just $HOME/.vscode/mcp.json and $HOME/.cursor/mcp.json for the non-datamate pass is two stat calls rather than a tree walk. Your call whether that is worth the branch; if you keep the current behavior, the comment reads narrower than what the code does, since it declines the whole sync rather than only the heal.
3. Nit — seen in collectDatamateHealPaths dedups on the raw path while scope is decided canonically, so one physical file reachable under two lexical paths gets queued twice. Scope agrees and the heal is idempotent, so the cost is a redundant read.
Thanks for the claims contract and the residuals list — being able to test numbered assertions instead of inferring intent is what made the last two rounds quick and specific.
Issue for this PR
Closes #1082
Type of change
What does this PR do?
Fixes the bug where
datamate-cli.jssuddenly opens as an editor tab when launching sessions, and the datamate MCP server dies with-32000 Connection closed.On desktop editors the extension-written
.vscode/mcp.jsondatamatestdio entry hascommand= the editor's Electron binary andenv: {"ELECTRON_RUN_AS_NODE": "1"}(Electron only runs the script as Node with that flag; without it, the editor GUI boots and opens the script as a document).datamate_manager addreused the entry's command + args but dropped theenvblock, both in the immediate spawn and in the entry persisted to.altimate-code/altimate-code.json— so the file popped onaddand again on every later session launch, with no self-repair in TUI/run(the healing sync only ran onserveboot).Changes:
readDatamateTransportFromIdenow returns the IDE entry's env (minusALTIMATE_EXTENSION_RPC, mirroring the sync path) andupdatedAt;handleAddcarries the env into the runtime MCP config and persists it asenvironment, plusupdatedAton disk so the sync recognizes the entry as current.extractSpawnEnvironmenthelper so the two paths stay in lockstep.runnow runsyncDatamateUrlFromVscodeMcpbefore the first session, asservealready did — entries already persisted broken in the field self-heal on the next launch. The heal is scoped to the containing git project root (resolveDatamateSyncRoot, bounded at the home directory), walks the config files from the launch directory up to that root the way the loader does (a nested package's own config is healed too), and in the worker it is sequenced strictly before config load, the first in-process request, andServer.listen, so the first session connects with the healed entry rather than a stale cached one.datamate_manager addon an existing-but-disconnected entry likewise refreshes it from the current IDE transport before connecting.ELECTRON_RUN_AS_NODEonly, since the carried env is spread over the host process env at spawn. Transport sources are only the two locations the extension writes (**/.vscode/mcp.json,**/.cursor/mcp.json), parsed through a validatingparseIdeTransport(local needs a non-emptycommand, remote a non-emptyurl; blank tombstones and incomplete entries are skipped rather than winning selection — an incomplete entry used to be persisted as a url-lessremote). Entries derived from an IDE file carry provenance (managedBy: "altimate-ide"+sourceMcpJson), and the boot heal never rewrites a global entry from a project file unless that entry's stamp matches the exact IDE file — hand-added and legacy global entries are left alone; an explicitdatamate_manager addis what (re)stamps them and is the remedy for a legacy global entry.syncDatamateUrlFromVscodeMcphas a second pass that refreshes the URL (andupdatedAt) of other remote MCP entries mirrored from the IDE config (name match, URL differs). That pass is not new behavior —serveboot has always run it — TUI/runnow just apply the same refresh consistently. Spawn/env behavior for non-datamate servers is unchanged.How did you verify your code works?
E2E in the docker code-server harness against a desktop-shaped mcp.json entry (command = an Electron-contract shim that opens its args as documents unless
ELECTRON_RUN_AS_NODE=1), driven through realrunsessions:datamate_manager add(project)-32000 Connection closed, env-less entry persistedconnected as 'datamate', entry carriesenvironment+ provenanceaddbelow)datamate_manager add --scope globalon that legacy entry, then relaunch.cursor/mcp.jsontombstone sorting firstUnit tests:
test/release-validation/mcp-datamate-stdio-env.test.tscovers the env carry (strip rule, omission when empty, back-compat bare shape, non-string filtering) and sync parity. Existingmcp-datamate-893suite unchanged and green;tsgo --noEmitclean.Screenshots / recordings
Before —
datamate_manager addpops the file open:After — same broken persisted entry, next session heals it and nothing pops:
Checklist
Review contract (claims + residuals) lives in the pinned review-log comment.